Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade eslint #8101

Closed
wants to merge 34 commits into from
Closed

Upgrade eslint #8101

wants to merge 34 commits into from

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Aug 26, 2016

Fixes #7609
Fixes #7622

Upgrades eslint and other related dependencies to the latest versions so that we can take advantage of some of the new auto-fixing features while making the transition to babel 6.

The commits are laid out to help the reviewer focus on a single change type at a time. commits mentioning "autofix" were done completely with npm run lintroller and do not need to be as thoroughly checked.

@spalger spalger added the review label Aug 26, 2016
@spalger
Copy link
Contributor Author

spalger commented Aug 26, 2016

The majority of the rules were fixable with eslint alone, but no-unused-vars was a bit trickier. Since we had so many violations, and I didn't want to go numb fixing them all by hand, I wrote some jscodeshift transforms to help with common scenarios: https:/kreeware/kibana-jscodeshift/tree/master/transforms

@spalger spalger force-pushed the upgrade/eslint branch 8 times, most recently from 828490b to 4b43cdc Compare August 31, 2016 00:04
@cjcenizal cjcenizal self-assigned this Sep 1, 2016
'src',
'tasks',
'test',
'utilities',
Copy link
Contributor

@cjcenizal cjcenizal Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This array needs to be defined with the root and a globbing pattern for lint_staged_files.js to pick up the staged files:

    lintThese: [
      '<%= root %>/Gruntfile.js',
      '<%= root %>/bin/**/*.js',
      '<%= root %>/config/**/*.js',
      '<%= root %>/src/**/*.js',
      '<%= root %>/tasks/**/*.js',
      '<%= root %>/test/**/*.js',
      '<%= root %>/utilities/**/*.js',
    ],

Copy link
Contributor Author

@spalger spalger Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the **/*.js is necessary. Makes sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use ** instead of the root reference, too:

    lintThese: [
      '**/Gruntfile.js',
      '**/bin/**/*.js',
      '**/config/**/*.js',
      '**/src/**/*.js',
      '**/tasks/**/*.js',
      '**/test/**/*.js',
      '**/utilities/**/*.js',
    ],

@cjcenizal
Copy link
Contributor

cjcenizal commented Sep 1, 2016

This is really awesome. I found one bug, and have a few other thoughts:

  1. Where are the eslint rule changes documented? Can we add some eslint-config-kibana 0.2.0 release notes? This way I will know what to test for.
  2. Per @thomasneirynck 's point, if you fix some linting errors but don't commit the file, then the lint task will pass and we'll end up committing files that violate our linting rules. I guess these should be caught by Jenkins, though, right? The other solution I can think of is to do some sort of check in our linting task: if a file is staged, but has unstaged changes, error out and require the user to stage the changes or unstage the file.

@spalger
Copy link
Contributor Author

spalger commented Sep 2, 2016

an we add some eslint-config-kibana 0.2.0 release notes?

Eh, there is less than a single page of commits. Release notes are probably overkill

if you fix some linting errors but don't commit the file, then the lint task will pass and we'll end up committing files that violate our linting rules

Unfortunately, preventing this would require a pretty sophisticated grunt task. This limitation has existed since the pre-commit check was introduced years ago and only comes up very rarely, so I'm not convinced it's worth the effort.

@spalger spalger force-pushed the upgrade/eslint branch 12 times, most recently from f4b5e12 to 9fb001c Compare September 7, 2016 01:17
@cjcenizal
Copy link
Contributor

LGTM

@spalger
Copy link
Contributor Author

spalger commented Dec 2, 2016

I'm going to take another stab at this in a new pr, with a basic tool to help transform files

@spalger spalger closed this Dec 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.eslintignore file is superseded by "lintThese" config var in Gruntfile ESLint should catch unused vars
3 participants